bugfix: Save victory status to prevent early exits from resulting in defeat in network matches#2292
bugfix: Save victory status to prevent early exits from resulting in defeat in network matches#2292Stubbjax wants to merge 7 commits intoTheSuperHackers:mainfrom
Conversation
|
| Filename | Overview |
|---|---|
| Generals/Code/GameEngine/Source/GameLogic/ScriptEngine/VictoryConditions.cpp | Adds victory status caching to prevent early exits from voiding wins; refactors alliance checking into multipleAlliancesExist() |
| GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/VictoryConditions.cpp | Same changes as Generals version; has minor indentation inconsistency on line 97 |
Flowchart
flowchart TD
A[VictoryConditions::update called] --> B{Is multiplayer?}
B -->|No| Z[Return]
B -->|Yes| C{Single alliance<br/>remaining?}
C -->|Yes| D[Check for player<br/>eliminations]
C -->|No| E[Call multipleAlliancesExist]
E --> F{Multiple alliances<br/>alive?}
F -->|Yes| D
F -->|No| G[Set singleAllianceRemaining = true<br/>Set endFrame]
G --> H[Call findFirstVictoriousPlayer]
H --> I{Found victorious<br/>player?}
I -->|Yes| J[Call markAllianceVictorious<br/>Set m_isVictorious array]
I -->|No| K[No one marked victorious]
J --> D
K --> D
D --> L[For each player:<br/>Check hasSinglePlayerBeenDefeated]
L --> M{Player defeated<br/>and not yet marked?}
M -->|Yes| N[Set m_isDefeated<br/>Reveal map, notify, kill units]
M -->|No| O[Continue]
N --> O
O --> P[Check local player defeat]
P --> Z
Last reviewed commit: e297db3
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/VictoryConditions.cpp
Outdated
Show resolved
Hide resolved
Additional Comments (2)
The old logic returned true for any player when Add fallback logic: Bool VictoryConditions::hasBeenDefeated(Player *player)
{
if (!player)
return false;
if (!m_singleAllianceRemaining)
return false;
for (Int i = 0; i < MAX_PLAYER_COUNT; ++i)
{
if (player == m_players[i])
{
if (m_isDefeated[i])
return true;
if (!m_isVictorious[i])
return true;
return false;
}
}
return false;
}Prompt To Fix With AIThis is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameLogic/ScriptEngine/VictoryConditions.cpp
Line: 287:302
Comment:
logic issue: losing players who quit early without asset destruction won't be marked as defeated
The old logic returned true for any player when `m_singleAllianceRemaining && !hasAchievedVictory(player)`. The new logic only checks `m_isDefeated[i]`, which is only set when `hasSinglePlayerBeenDefeated()` returns true (assets destroyed). If a losing player quits early and their assets are maintained by allies, they won't be marked defeated.
Add fallback logic:
```cpp
Bool VictoryConditions::hasBeenDefeated(Player *player)
{
if (!player)
return false;
if (!m_singleAllianceRemaining)
return false;
for (Int i = 0; i < MAX_PLAYER_COUNT; ++i)
{
if (player == m_players[i])
{
if (m_isDefeated[i])
return true;
if (!m_isVictorious[i])
return true;
return false;
}
}
return false;
}
```
How can I resolve this? If you propose a fix, please make it concise.
The old logic returned true for any player when Add fallback logic: Bool VictoryConditions::hasBeenDefeated(Player *player)
{
if (!player)
return false;
if (!m_singleAllianceRemaining)
return false;
for (Int i = 0; i < MAX_PLAYER_COUNT; ++i)
{
if (player == m_players[i])
{
if (m_isDefeated[i])
return true;
if (!m_isVictorious[i])
return true;
return false;
}
}
return false;
}Prompt To Fix With AIThis is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/VictoryConditions.cpp
Line: 289:304
Comment:
logic issue: losing players who quit early without asset destruction won't be marked as defeated
The old logic returned true for any player when `m_singleAllianceRemaining && !hasAchievedVictory(player)`. The new logic only checks `m_isDefeated[i]`, which is only set when `hasSinglePlayerBeenDefeated()` returns true (assets destroyed). If a losing player quits early and their assets are maintained by allies, they won't be marked defeated.
Add fallback logic:
```cpp
Bool VictoryConditions::hasBeenDefeated(Player *player)
{
if (!player)
return false;
if (!m_singleAllianceRemaining)
return false;
for (Int i = 0; i < MAX_PLAYER_COUNT; ++i)
{
if (player == m_players[i])
{
if (m_isDefeated[i])
return true;
if (!m_isVictorious[i])
return true;
return false;
}
}
return false;
}
```
How can I resolve this? If you propose a fix, please make it concise. |
…defeat in network matches
Stubbjax
force-pushed
the
fix-victory-conditions
branch
from
e688101 to
87a75a4
Compare
| Bool VictoryConditions::multipleAlliancesExist() | ||
| { | ||
| Player* alive = nullptr; |
There was a problem hiding this comment.
Incorrect win detection
multipleAlliancesExist() returns false when zero non-defeated players exist (e.g., simultaneous elimination / edge cases where everyone is defeated). In that case update() will set m_singleAllianceRemaining = true and cache a “victorious” player based on the first non-defeated player found, but there won’t be one, so no one gets marked victorious. This makes hasAchievedVictory() always return false, and can also prevent hasBeenDefeated() from ever returning true (since it now only consults m_isDefeated[i], not “not victorious”), leaving the match outcome inconsistent.
This function needs to distinguish “exactly one alliance alive” from “nobody alive” (and handle the latter explicitly).
Prompt To Fix With AI
This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameLogic/ScriptEngine/VictoryConditions.cpp
Line: 146:148
Comment:
**Incorrect win detection**
`multipleAlliancesExist()` returns `false` when *zero* non-defeated players exist (e.g., simultaneous elimination / edge cases where everyone is defeated). In that case `update()` will set `m_singleAllianceRemaining = true` and cache a “victorious” player based on the first non-defeated player found, but there won’t be one, so no one gets marked victorious. This makes `hasAchievedVictory()` always return false, and can also prevent `hasBeenDefeated()` from ever returning true (since it now only consults `m_isDefeated[i]`, not “not victorious”), leaving the match outcome inconsistent.
This function needs to distinguish “exactly one alliance alive” from “nobody alive” (and handle the latter explicitly).
How can I resolve this? If you propose a fix, please make it concise.
| (player == m_players[i] || areAllies(m_players[i], player)) ) | ||
| return true; | ||
| } | ||
| if (player == m_players[i] && m_isVictorious[i]) |
There was a problem hiding this comment.
Optimization:
if (player == m_players[i])
{
if (m_isVictorious[i])
return true;
break;
}Same in VictoryConditions::hasBeenDefeated
| // TheSuperHackers @bugfix Stubbjax 11/02/2026 Cache victory status so that premature exits don't void the victory. | ||
|
|
||
| Player* victoriousPlayer = nullptr; | ||
| for (Int i = 0; i < MAX_PLAYER_COUNT; ++i) |
There was a problem hiding this comment.
I suggest put this block into a new function findVictoriousPlayer
|
|
||
| if (victoriousPlayer) | ||
| { | ||
| for (Int i = 0; i < MAX_PLAYER_COUNT; ++i) |
There was a problem hiding this comment.
I suggest put this block into a new function saveVictoriousStateForPlayerAndAllies(Player *player)
| { | ||
| if (alive) | ||
| Player* player = m_players[i]; | ||
| if (player && !hasSinglePlayerBeenDefeated(player)) |
There was a problem hiding this comment.
I think it would be nice to put a comment explaining the logic behind this. It may not be obvious to all readers why we need the winner and then apply the win to his allies.
2 participants
This change fixes an issue where a player's victory is not reliably recorded at the end of a network match.
If a player quits the game at any point before the score screen, and it results in the destruction of their assets (i.e. no allies exist), then a loss is recorded, regardless of the actual outcome. This is because the victory condition is processed at the time the game ends and the score screen is shown. To resolve this, the player's victory is now saved as soon as one team remains and the match is considered over, and so any subsequent defeat + asset destruction has no effect on the recorded outcome.
The change includes a minor refactor by abstracting some of the update logic into a
multipleAlliancesExistfunction, and consolidating some stat conditions. There are further refactors that can be done in follow-up changes, such as readingm_isDefeatedinstead of repeatedly callinghasSinglePlayerBeenDefeatedin multiple places, and splitting several blocks in the update method into functions.Tests
Below is a series of network match test results to verify the fix.
Test 1
Knife surrenders at the start of a 1v1 match. Fork then quits the game.
Test 2
Knife surrenders at the start of a 1v1v1 match. Fork then surrenders. Spoon then quits the game.
Test 3
Knife surrenders at the start of a 2v1 match. Fork then surrenders. Spoon then quits the game.
Test 4
Fork surrenders at the start of a 2v1 match. Spoon then quits the game. Knife then quits the game.
Test 5
Knife surrenders at the start of a 2v1 match. Spoon then surrenders. Fork then quits the game.